Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hf landscape updates [WIP] #3193

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Conversation

mdietze
Copy link
Member

@mdietze mdietze commented Jul 10, 2023

Description

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

} else {
PEcAn.logger::logger.error(samples.file, "not found, this file is required by the run.write.configs function")
### use.existing.samples must be a filename
load(use.existing.samples)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a doubletake here ("wait, why is it calling load on a bool?") even after reading at the top of the function that use.existing.samples could be a filepath.

Possible alternate design: Call this param existing.sample.file, have the argument always be a path, and make the default value be NULL (behaving like FALSE does now). Then passing existing.sample.file = "samples.Rdata" would behave like TRUE does now with no need for a special case.

Comment on lines +45 to +51
# samples <- new.env()
# load(samples.file, envir = samples) ## loads ensemble.samples, trait.samples, sa.samples, runs.samples, env.samples
# trait.samples <- samples$trait.samples
# ensemble.samples <- samples$ensemble.samples
# sa.samples <- samples$sa.samples
# runs.samples <- samples$runs.samples
# ## env.samples <- samples$env.samples
Copy link
Member

@infotroph infotroph Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing this is temporarily commented out while developing the replacement, but I strongly recommend all replacement load calls copy the pattern shown here of loading all Rdata files into their own environment -- not only does it keep the package checks from complaining about variables with no visible binding, it means there's no ambiguity about what names were loaded from which file.

The hack of reading the file into an environment and then immediately assigning back out to separate variables was just to minimize lines changed while quieting the check note. I'd support a further refactor to use all of these directly from thee loading environment instead (e.g. write all references to sa.samples as samples$sa.samples)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be honest, this was me trying to resolve a merge conflict and not having time to go back and check the code in more detail (it's been months since I worked on this PR). My plan had been to take a closer look at this function and once I had pulled in the latest code

Comment on lines +365 to +369
tmp = list()
for(i in seq_len(as.numeric(settings$state.data.assimilation$chains))){
tmp[[i]] = MCMC_data
}
MCMC_data = tmp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this equivalent to MCMC_data = rep(list(MCMC_data), settings$state.data.assimilation$chains)? That form would make it clearer to me that this is a single list of identical copies of the same list.
I haven't tested, but rep is probably more memory efficient too, because it can be allocated once and doesn't need to grow

@infotroph infotroph removed the Tests label Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants